Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read certificate file ref from DCOD file #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

metsma
Copy link
Contributor

@metsma metsma commented Mar 3, 2025

WE2-1066

Fixes: #119

Signed-off-by: Raul Metsma raul@metsma.ee

@metsma metsma force-pushed the lat branch 11 times, most recently from b777700 to d22d90e Compare March 5, 2025 07:01
Fixes: web-eid#119

Signed-off-by: Raul Metsma <raul@metsma.ee>
Copy link
Member

@mrts mrts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all very nice code, was a pleasure to read - thanks for a job well done!

tag = *begin++;
if ((tag & 0x1F) == 0x1F) { // Multi-byte tag
if (!*this) {
throw std::invalid_argument("Invalid TLV: Unexpected end of tag");
Copy link
Member

@mrts mrts Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use THROW() to get context info into the log, also the error message could be a little more precise:

Suggested change
throw std::invalid_argument("Invalid TLV: Unexpected end of tag");
THROW(std::invalid_argument, "Invalid TLV: Multi-byte tag without second byte");

}

if (!*this) {
throw std::invalid_argument("Invalid TLV: Missing length field");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use THROW() here as well.

if (length & 0x80) { // Extended length encoding
auto num_bytes = uint8_t(length & 0x7F);
if (num_bytes == 0 || num_bytes > 4 || std::distance(begin, end) < num_bytes) {
throw std::invalid_argument("Invalid TLV: Incorrect extended length encoding");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use THROW() here as well.

}

if (std::distance(begin, end) < length) {
throw std::invalid_argument("Invalid TLV: Insufficient value data");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use THROW() here as well.

{
for (; tlv; ++tlv) {
if (tlv.tag == tag) {
if constexpr (sizeof...(tags) > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our coding standard requires using curly braces with if:

Suggested change
if constexpr (sizeof...(tags) > 0)
if constexpr (sizeof...(tags) > 0) {

namespace electronic_id
{

struct TLV
Copy link
Member

@mrts mrts Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a brief explanation that the constructor consumes the tag and length so that begin and end point to the value directly:

Suggested change
struct TLV
/**
* Represents a single Tag-Length-Value structure used in DER-encoded eID card files.
*
* The constructor parses the tag and length from the provided byte range,
* then adjusts its iterators so that `begin` and `end` reference only the value bytes.
* If the TLV is empty, `operator bool()` returns false.
*/
struct TLV

KeyInfo authKeyRef() const override;
KeyInfo signKeyRef() const override;

const byte_vector& readEF_File(byte_vector file, std::map<byte_vector, byte_vector> &cache) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs reformatting with clang-format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error response: '006282', protocol 2 in src/SmartCard.cpp
2 participants